Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

removed unneeded plugin update #944

Merged
merged 5 commits into from
Jul 30, 2021
Merged

removed unneeded plugin update #944

merged 5 commits into from
Jul 30, 2021

Conversation

jennuine
Copy link
Contributor

🦟 Bug fix

Summary

When a plugin was added an Update was called immediately. With #497 updates are also being called in a loop, which was causing deadlocks since both could be called at the same time. This PR removes the initial update (after the plugin is inserted) since updates are already happening periodically.

Checklist

  • Signed all commits for DCO
  • Added tests
  • Updated documentation (as needed)
  • Updated migration guide (as needed)
  • codecheck passed (See contributing)
  • All tests passed (See test coverage)
  • While waiting for a review on your PR, please help review another open pull request to support the maintainers

Note to maintainers: Remember to use Squash-Merge

@jennuine jennuine requested a review from chapulina July 30, 2021 01:18
@github-actions github-actions bot added the 🏰 citadel Ignition Citadel label Jul 30, 2021
@chapulina chapulina added the bug Something isn't working label Jul 30, 2021
chapulina
chapulina approved these changes Jul 30, 2021
View reviewed changes
Copy link
Contributor

@chapulina chapulina left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for cleaning this up! Works for me. Let's just wait for CI results.

iche033
iche033 reviewed Jul 30, 2021
View reviewed changes
Copy link
Contributor

@iche033 iche033 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ran into this issue on ign-gazebo4 so ported the fix over and confirmed that the issue is fixed.

JointPositionController tests are failing on CI though

@chapulina
wait for update
20cdc9f 
Signed-off-by: Louise Poubel <[email protected]>
@chapulina
Copy link
Contributor

JointPositionController tests are failing on CI though

Should be fixed by waiting for updates in 20cdc9f

@codecov
Copy link

codecov bot commented Jul 30, 2021 •
edited
Loading

Codecov Report

Merging #944 (20cdc9f) into ign-gazebo3 (638e97c) will decrease coverage by 0.04%.
The diff coverage is 0.00%.

Impacted file tree graph

@@               Coverage Diff               @@
##           ign-gazebo3     #944      +/-   ##
===============================================
- Coverage        77.84%   77.80%   -0.05%     
===============================================
  Files              220      220              
  Lines            12589    12579      -10     
===============================================
- Hits              9800     9787      -13     
- Misses            2789     2792       +3
Impacted Files Coverage Δ
src/gui/Gui.cc 67.20% <ø> (+0.27%) ⬆️
src/gui/GuiRunner.cc 94.64% <0.00%> (+0.89%) ⬆️
src/gui/plugins/plot_3d/Plot3D.cc 43.47% <0.00%> (-4.35%) ⬇️
...int_position_controller/JointPositionController.cc 62.28% <0.00%> (+1.14%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 638e97c...20cdc9f. Read the comment docs.

@chapulina chapulina merged commit 59a2faf into ign-gazebo3 Jul 30, 2021
@chapulina chapulina deleted the jennuine/remove_plugin_update branch July 30, 2021 06:14
@chapulina chapulina mentioned this pull request Sep 15, 2021
Closed
18 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iche033 iche033 iche033 left review comments

@chapulina chapulina chapulina approved these changes

Assignees
No one assigned
Labels
bug Something isn't working 🏰 citadel Ignition Citadel
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@jennuine @chapulina @iche033